Conversation
tolusha
requested review from
akurinnoy,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tolusha
changed the title
|
The main problem here is that even if we mount the resources (marked with special attributes) only when the workspace starts—and not while it’s running—we still need to decide how to handle changes to those resources while the workspace is active. |
|
I haven't tried, but is it possible to ignore configmaps/secrets that have the |
|
It slightly mitigates the problem, but it doesn’t address it completely. For example:
|
|
There is a solution that I don’t really like, but it could work. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughController now conditionally fetches the workspace Deployment and threads workspace runtime context (running vs not) plus the Deployment into automount provisioning. Automount handlers filter candidate ConfigMaps, Secrets, and PVCs by a new Changes
Sequence DiagramsequenceDiagram
participant Controller
participant DeploymentAPI as Deployment<br/>Fetcher
participant Automount
participant Handlers as ConfigMap/Secret/PVC<br/>Handlers
participant Filter as Mount-on-start<br/>& Presence Check
Controller->>DeploymentAPI: GetClusterDeployment(workspace) (if running)
DeploymentAPI-->>Controller: workspaceDeployment or nil
Controller->>Automount: ProvisionAutoMountResourcesInto(..., workspaceDeployment)
Automount->>Handlers: getDevWorkspaceConfigmaps(..., workspaceDeployment)
Automount->>Handlers: getDevWorkspaceSecrets(..., workspaceDeployment)
Automount->>Handlers: getAutoMountPVCs(..., workspaceDeployment)
loop for each candidate resource
Handlers->>Filter: isAllowedToMount(resource, workspaceDeployment)
alt resource has mount-on-start
Filter->>Filter: if workspaceDeployment == nil => allow
Filter->>Filter: else check volume/env presence in deployment
Filter-->>Handlers: include only if present
else
Filter-->>Handlers: include
end
Handlers-->>Automount: allowed Resources
end
Automount-->>Controller: aggregated volumes & volumeMounts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provision/automount/gitconfig.go (1)
53-76:⚠️ Potential issue | 🟠 MajorDo not mount empty synthetic git resources after filtering all inputs.
When a first git credential/TLS resource is added with
mount-on-startwhile the workspace is running, the helpers can filter out every input, but lines 63-74 still sync and return the merged Secret/ConfigMap mounts. That changes the Deployment with empty synthetic git config and can restart the workspace. Have the helpers report whether any eligible input was included, and skip returning these synthetic mounts unless they are already present inworkspaceDeploymentor contain eligible/base config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/gitconfig.go` around lines 53 - 76, The code currently always syncs and mounts synthetic git Secret/ConfigMap even when helpers filtered out all inputs; update mergeGitCredentials and constructGitConfig (or their callers) to return a boolean indicating whether any eligible inputs or base config/tls were included, then in this block only call sync.SyncObjectWithCluster and include the results from getAutomountSecret/getAutomountConfigmap in flattenAutomountResources if that boolean is true OR the corresponding resource already exists in workspaceDeployment or contains non-empty/base config; otherwise skip syncing and do not return the synthetic mounts. Ensure you reference mergeGitCredentials, constructGitConfig, sync.SyncObjectWithCluster, getAutomountSecret, getAutomountConfigmap and workspaceDeployment when implementing the conditional logic.
🧹 Nitpick comments (3)
pkg/provision/automount/common.go (1)
412-422: Nit: loop variables are pluralized but represent single items.
automountVolumes/deploymentVolumesare singular elements produced by ranging over slices. Renaming improves readability.Proposed rename
-func isVolumeMountExistsInDeployment(automountResource Resources, workspaceDeployment *appsv1.Deployment) bool { - for _, automountVolumes := range automountResource.Volumes { - for _, deploymentVolumes := range workspaceDeployment.Spec.Template.Spec.Volumes { - if automountVolumes.Name == deploymentVolumes.Name { +func isVolumeMountExistsInDeployment(automountResource Resources, workspaceDeployment *appsv1.Deployment) bool { + for _, automountVolume := range automountResource.Volumes { + for _, deploymentVolume := range workspaceDeployment.Spec.Template.Spec.Volumes { + if automountVolume.Name == deploymentVolume.Name { return true } } } return false }Also, function names like
isVolumeMountExistsInDeployment/isEnvFromSourceExistsInDeploymentread awkwardly — considervolumeExistsInDeployment/envFromSourceExistsInDeployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/common.go` around lines 412 - 422, Rename the pluralized loop variables to singulars and consider clearer function names: in isVolumeMountExistsInDeployment change automountVolumes -> automountVolume and deploymentVolumes -> deploymentVolume; similarly rename variables in isEnvFromSourceExistsInDeployment if present. Also consider renaming the functions to volumeExistsInDeployment and envFromSourceExistsInDeployment (update all call sites) to improve readability while preserving behavior.pkg/provision/automount/templates.go (1)
96-98: Vacuous-truth and nesting readability — small simplification possible.
!slices.ContainsFunc(xs, func(x) bool { return !p(x) })reads as a double negation of "all satisfy p". It also returnstruefor an empty slice, which is fine here only because the loop below then doesn't iterate. Consider using a helper or an explicit all-loop for clarity, especially since the same pattern is repeated inmergeGitCredentials(Line 159-161):Optional readability tweak
allConfigMapsMountOnStart := len(certificatesConfigMaps) > 0 for i := range certificatesConfigMaps { if !isMountOnStart(&certificatesConfigMaps[i]) { allConfigMapsMountOnStart = false break } }or, if you prefer keeping
slices:allConfigMapsMountOnStart := len(certificatesConfigMaps) > 0 && !slices.ContainsFunc(certificatesConfigMaps, func(cm corev1.ConfigMap) bool { return !isMountOnStart(&cm) })Not blocking — the behavior is correct today because empty slices short-circuit via the surrounding loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/templates.go` around lines 96 - 98, Replace the double-negation slices.ContainsFunc pattern used to compute allConfigMapsMountOnStart with a clearer "all" check: iterate certificatesConfigMaps (or use len(certificatesConfigMaps) > 0 && ...) and set allConfigMapsMountOnStart to false on the first element for which isMountOnStart(&cm) is false; apply the same clearer pattern in the mergeGitCredentials block where the identical slices.ContainsFunc(...) negation is used so the logic and empty-slice semantics remain explicit and readable.pkg/provision/automount/common_test.go (1)
413-710: Duplicate test coverage — three pairs assert the exact same scenario.These pairs pass identical arguments (
false, true, emptyDeployment()) against the same fake object and assert identical outcomes:
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted(Line 413) ↔TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment(Line 612)TestShouldNotMountConfigMapWithMountOnStartIfWorkspaceStarted(Line 452) ↔TestShouldNotMountConfigMapWithMountOnStartWhenRunningAndNotInDeployment(Line 530)TestShouldNotMountPVCWithMountOnStartIfWorkspaceStarted(Line 491) ↔TestShouldNotMountPVCWithMountOnStartWhenRunningAndNotInDeployment(Line 694)Recommend collapsing each pair into a single test (or making one of them genuinely test a different state, e.g.
isWorkspaceRunning=truewithworkspaceDeployment=nil, which currently has no coverage and is the ambiguous case discussed incommon.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/common_test.go` around lines 413 - 710, Duplicate tests cover the same inputs and assertions: collapse each pair into one test (e.g., remove either TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted or TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment, same for the ConfigMap and PVC pairs) so you only call ProvisionAutoMountResourcesInto once per scenario; alternatively change one test of each pair to exercise the ambiguous case described in common.go by calling ProvisionAutoMountResourcesInto(testPodAdditions, testAPI, testNamespace, false, true, nil) (i.e., isWorkspaceStarted/isWorkspaceRunning=true with workspaceDeployment=nil) and adjust assertions accordingly; update or remove redundant test helper usage (mountOnStartSecretAsFile, mountOnStartConfigMapAsFile, mountOnStartPVC) so each remaining test uniquely covers either the "workspace started" or the "running and not in deployment" scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/workspace/devworkspace_controller.go`:
- Around line 459-467: The current branch around isWorkspaceRunning is inverted:
it only calls wsprovision.GetClusterDeployment when the workspace is NOT
running, leaving workspaceDeployment nil for running workspaces and causing
automount/isAllowedToMount to allow unsafe mounts. Change the condition to fetch
the existing deployment when isWorkspaceRunning is true by calling
wsprovision.GetClusterDeployment(workspace, clusterAPI) and assigning
workspaceDeployment (and returning errors as before) so
automount/isAllowedToMount receives the actual deployment for running
workspaces.
In `@pkg/constants/attributes.go`:
- Around line 175-179: The exported annotation constant MountOnStartAttribute
currently uses "controller.devfile.io/mount-on-start" but the PR and docs expect
"controller.devfile.io/mount-on-start-only"; update the value of
MountOnStartAttribute to "controller.devfile.io/mount-on-start-only" and then
search/replace all usages (tests, docs, and any code references) to use
MountOnStartAttribute so the public API key is consistent across code, tests,
and documentation.
In `@pkg/provision/automount/common_test.go`:
- Line 413: Rename the test function
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted to correct the typo
(MountOnStart) so the function name reads
TestShouldNotMountSecretWithMountOnStartIfWorkspaceStarted; update any
references or subtests that call or document this function name to match the
corrected identifier (e.g., in package tests, test runners, or comments) to
ensure compilation and clarity.
- Around line 712-739: The test constructs a Deployment volume using the raw
string "test-pvc" but the production helper common.AutoMountPVCVolumeName(...)
is used to name volumes; update the test to call common.AutoMountPVCVolumeName
on the PVC name when creating the deployment volume and in the final assertion
so the test uses the same naming logic as ProvisionAutoMountResourcesInto and
getAutoMountPVCs; locate the volume creation in
TestMountOnStartPVCAllowedWhenVolumeExistsInDeployment and replace the literal
"test-pvc" with common.AutoMountPVCVolumeName("test-pvc") and also assert
equality against common.AutoMountPVCVolumeName("test-pvc") in the assert.Equal
call.
In `@pkg/provision/automount/common.go`:
- Around line 408-422: The current name-only check in
isVolumeMountExistsInDeployment can miss "shape changes" (e.g., ConfigMap
switched from file volume to envFrom) causing the new representation to be
ignored; update the reconciliation checks so that when a resource name matches
an existing deployment volume but the new automount Resources describe it as an
EnvFrom (or vice versa) we detect the mismatch and treat it as an
existing-but-changed resource that requires a restart/reconcile. Concretely,
augment the logic around isVolumeMountExistsInDeployment and
isEnvFromSourceExistsInDeployment (or the higher-level existsInDeployment /
isAllowedToMount flow) to: 1) detect same underlying resource name present in
deployment volumes but absent as a volume in automount.Resources (and check
container.EnvFrom entries for the same name), 2) return a sentinel
"shape-changed" result or treat it as exists so the controller triggers the
restart/reconcile, and 3) add a unit test covering the transition from
mount-as:file → mount-as:env (and vice versa) to ensure the change is not
silently ignored.
- Around line 378-401: The gating logic in isAllowedToMount is inverted: when
the workspace is running we must check the existing deployment, but the code
currently does the opposite; update the conditional so that the branch that
calls existsInDeployment(automountResource, workspaceDeployment) runs only when
isWorkspaceRunning is true (i.e., change the check around isWorkspaceRunning),
ensuring isMountOnStart is respected for running workspaces and
workspaceDeployment is used to gate mounts.
In `@pkg/provision/automount/configmap.go`:
- Around line 62-67: When a running workspace has an existing mount-on-start
ConfigMap, the current logic builds a new shape via getAutomountConfigmap(...)
and then drops it if isAllowedToMount(...) can't find an exact match, causing
the old mount to be removed and triggering a rollout; instead, detect that
workspaceDeployment already contains the mounted resource and preserve its
existing volume/mount/envFrom into the desired spec: if isAllowedToMount(...)
fails for a mount-on-start resource while isWorkspaceRunning is true, copy the
cluster-side resource (volume, volumeMount, and envFrom entries) from
workspaceDeployment into the automountCM entry added to allAutoMountResources
(or change getAutomountConfigmap to return the preserved cluster-side resource),
and apply the same preservation pattern for Secret and PVC automount paths so
in-place shape changes don’t drop existing mounts.
In `@pkg/provision/automount/gitconfig_test.go`:
- Around line 407-434: The test
TestMountGitCredentialWhenMixedMountOnStartSecrets asserts behavior that
contradicts the comment on shouldIncludeGitObject; either document the contract
or change logic—here, add a clarifying comment inside
TestMountGitCredentialWhenMixedMountOnStartSecrets stating that
ProvisionGitConfiguration will allow mount-on-start secrets through when any
sibling secret lacks the mount-on-start annotation (per shouldIncludeGitObject
in templates.go), and that this is an intentional tradeoff (only
fully-mount-on-start sets are gated), so the test expects 2 volumes/2 mounts for
the mixed case; reference shouldIncludeGitObject and ProvisionGitConfiguration
in the comment for future readers.
- Around line 224-251: The test initializes sync.ClusterAPI without setting
ClusterAPI.Ctx, leaving it nil and diverging from production; set ClusterAPI.Ctx
to a real context (e.g., context.Background() or context.TODO()) in the test
setup where ClusterAPI is constructed so getGitResources() and
sync.SyncObjectWithCluster() receive a non-nil context; update the ClusterAPI
construction in this test (and the other tests noted) so clusterAPI.Ctx is
assigned before calling ProvisionGitConfiguration or any API client operations.
In `@pkg/provision/automount/templates.go`:
- Around line 183-201: The current shouldIncludeGitObject logic short-circuits
on !allGitObjectsMountOnStart causing mount-on-start objects to be included
whenever any sibling is non-mount-on-start; fix by making that branch also
require the merged volume to already be present (i.e., require
workspaceDeployment != nil and/or isVolumeMountExistsInDeployment(...) to be
true) so a global "some sibling is non-mount-on-start" does not alone permit
inclusion, or alternatively update the comment to state the true behaviour;
change the condition involving allGitObjectsMountOnStart so it reads like
"!allGitObjectsMountOnStart && (workspaceDeployment != nil ||
isVolumeMountExistsInDeployment(automountMergedResource, workspaceDeployment))"
(or equivalent) and keep isMountOnStart, allGitObjectsMountOnStart,
workspaceDeployment, and isVolumeMountExistsInDeployment as the referenced
symbols.
---
Outside diff comments:
In `@pkg/provision/automount/gitconfig.go`:
- Around line 53-76: The code currently always syncs and mounts synthetic git
Secret/ConfigMap even when helpers filtered out all inputs; update
mergeGitCredentials and constructGitConfig (or their callers) to return a
boolean indicating whether any eligible inputs or base config/tls were included,
then in this block only call sync.SyncObjectWithCluster and include the results
from getAutomountSecret/getAutomountConfigmap in flattenAutomountResources if
that boolean is true OR the corresponding resource already exists in
workspaceDeployment or contains non-empty/base config; otherwise skip syncing
and do not return the synthetic mounts. Ensure you reference
mergeGitCredentials, constructGitConfig, sync.SyncObjectWithCluster,
getAutomountSecret, getAutomountConfigmap and workspaceDeployment when
implementing the conditional logic.
---
Nitpick comments:
In `@pkg/provision/automount/common_test.go`:
- Around line 413-710: Duplicate tests cover the same inputs and assertions:
collapse each pair into one test (e.g., remove either
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted or
TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment, same for
the ConfigMap and PVC pairs) so you only call ProvisionAutoMountResourcesInto
once per scenario; alternatively change one test of each pair to exercise the
ambiguous case described in common.go by calling
ProvisionAutoMountResourcesInto(testPodAdditions, testAPI, testNamespace, false,
true, nil) (i.e., isWorkspaceStarted/isWorkspaceRunning=true with
workspaceDeployment=nil) and adjust assertions accordingly; update or remove
redundant test helper usage (mountOnStartSecretAsFile,
mountOnStartConfigMapAsFile, mountOnStartPVC) so each remaining test uniquely
covers either the "workspace started" or the "running and not in deployment"
scenario.
In `@pkg/provision/automount/common.go`:
- Around line 412-422: Rename the pluralized loop variables to singulars and
consider clearer function names: in isVolumeMountExistsInDeployment change
automountVolumes -> automountVolume and deploymentVolumes -> deploymentVolume;
similarly rename variables in isEnvFromSourceExistsInDeployment if present. Also
consider renaming the functions to volumeExistsInDeployment and
envFromSourceExistsInDeployment (update all call sites) to improve readability
while preserving behavior.
In `@pkg/provision/automount/templates.go`:
- Around line 96-98: Replace the double-negation slices.ContainsFunc pattern
used to compute allConfigMapsMountOnStart with a clearer "all" check: iterate
certificatesConfigMaps (or use len(certificatesConfigMaps) > 0 && ...) and set
allConfigMapsMountOnStart to false on the first element for which
isMountOnStart(&cm) is false; apply the same clearer pattern in the
mergeGitCredentials block where the identical slices.ContainsFunc(...) negation
is used so the logic and empty-slice semantics remain explicit and readable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb85f151-8a4c-494e-833d-a35807facca0
📒 Files selected for processing (12)
controllers/workspace/devworkspace_controller.gopkg/constants/attributes.gopkg/provision/automount/common.gopkg/provision/automount/common_persistenthome_test.gopkg/provision/automount/common_test.gopkg/provision/automount/configmap.gopkg/provision/automount/gitconfig.gopkg/provision/automount/gitconfig_test.gopkg/provision/automount/pvcs.gopkg/provision/automount/secret.gopkg/provision/automount/templates.gopkg/provision/workspace/deployment.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
+ Coverage 35.48% 36.59% +1.11%
==========================================
Files 168 168
Lines 14484 14671 +187
==========================================
+ Hits 5139 5369 +230
+ Misses 9006 8953 -53
- Partials 339 349 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provision/automount/secret.go (1)
43-62:⚠️ Potential issue | 🟠 MajorApply the mount-on-start skip before validating skipped Secrets.
A newly created running-workspace Secret with
controller.devfile.io/mount-on-start=truecan still fail reconciliation viacheckAutomountVolumeForPotentialError()orgetAccessModeForAutomount()before Line 61 skips it. Since it is not being mounted into the running workspace, avoid surfacing validation errors until it is actually eligible to mount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/secret.go` around lines 43 - 62, Move the early "skip" check so Secrets that are not eligible to be mounted are bypassed before running validations: call isAllowedToMount(...) (using mountAs and mountPath from secret.Annotations or a lightweight/inference of automount target) and continue if it returns false, before invoking checkAutomountVolumeForPotentialError(...) or getAccessModeForAutomount(...). Only after isAllowedToMount returns true should you run checkAutomountVolumeForPotentialError, call getAccessModeForAutomount, and then build the full automountSecret via getAutomountSecret(...); this ensures validation errors are not surfaced for Secrets that will not be mounted.
♻️ Duplicate comments (3)
pkg/provision/automount/configmap.go (1)
62-67:⚠️ Potential issue | 🟠 MajorDropping not-yet-mounted
mount-on-startConfigMaps while running may remove existing mounts on shape changes.For a running workspace, if a
mount-on-startConfigMap is edited in a way that changes its computed shape (e.g.,mount-asswitched betweenenvandfile/subpath, or the configmap is renamed via recreation),getAutomountConfigmapreturns the new shape andisAllowedToMountchecks by volume/EnvFromSource name against the current Deployment. If the match fails, thiscontinuedrops the resource from the desired spec entirely, which then rolls the workspace — the opposite of the PR's intent.Matching by ConfigMap name (via
AutoMountConfigMapVolumeName) does mitigate the common case where onlymount-path/access-mode/subpathkeys change, but cross-mode transitions (volume ↔ EnvFromSource) are still a gap. Consider, formount-on-startresources that already exist in the Deployment, carrying forward the cluster-side volume/volumeMount/envFrom (rather than recomputing from the updated ConfigMap) so the desired spec matches what is running. The same applies to the Secret and PVC paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/configmap.go` around lines 62 - 67, When isAllowedToMount rejects the recomputed automount (from getAutomountConfigmap) for an existing running workspace, don't drop the resource; instead detect that the ConfigMap/Secret/PVC already exists in workspaceDeployment by matching on the cluster-side identifier (e.g., AutoMountConfigMapVolumeName for ConfigMaps) and carry forward the existing volume/volumeMount/envFrom from the Deployment into the desired automountCM so the desired spec matches the running shape; update the logic in pkg/provision/automount/configmap.go around getAutomountConfigmap and isAllowedToMount to: if isAllowedToMount returns false but a resource with the same cluster-side name exists in workspaceDeployment, copy the cluster-side volume/volumeMount/envFrom into automountCM (and do the same for Secret and PVC paths) instead of continue’ing, so cross-mode changes (volume ↔ envFrom) don’t remove existing mounts.pkg/provision/automount/gitconfig_test.go (1)
231-234:⚠️ Potential issue | 🟡 MinorInitialize
ClusterAPI.Ctxin the new test setups too.These new test
ClusterAPIinstances still leaveCtxnil, while the production path passesapi.Ctxinto client operations. AddCtx: context.Background()or a shared test constructor to keep fake-client tests aligned with production behavior.Also applies to: 257-259, 282-285, 306-309, 332-335, 367-370, 407-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/gitconfig_test.go` around lines 231 - 234, The test ClusterAPI instances (clusterAPI variables created as sync.ClusterAPI{ Client: fake.NewClientBuilder().WithObjects(&testSecret).Build(), Logger: zap.New(), ... }) leave Ctx nil; update each test setup (all occurrences around the clusterAPI initializations in gitconfig_test.go) to set Ctx: context.Background() or use a shared test constructor that returns a sync.ClusterAPI with Ctx populated, and ensure the test file imports context so client operations receive a non-nil context like production does.pkg/provision/automount/gitconfig.go (1)
98-115:⚠️ Potential issue | 🟠 MajorGate Git resources per source object, not per merged volume/list.
Lines 98-101 and 112-115 pass the entire Secret/ConfigMap list once the list-level helper returns true. That means a mount-on-start Git Secret/ConfigMap can still be merged into a running workspace when any sibling lacks the annotation, or when the shared generated volume already exists. Filter individual source objects, or track which source objects were already represented at workspace start, so newly created mount-on-start Git resources are not applied before restart.
Also applies to: 162-209
🧹 Nitpick comments (3)
pkg/provision/automount/pvcs.go (1)
24-24: Useappsv1alias for consistency with the rest of the package.The companion changes in
pkg/provision/automount/common.go(e.g.isAllowedToMount,existsInDeployment) reference the Deployment type asappsv1.Deployment. Importing the same package asv1here diverges from that convention and also collides visually withk8s.io/api/core/v1, which is aliasedcorev1on the next line. Aligning the alias will keep the package consistent and avoid future confusion.♻️ Proposed change
- v1 "k8s.io/api/apps/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1"And update the signature:
- workspaceDeployment *v1.Deployment, + workspaceDeployment *appsv1.Deployment,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/pvcs.go` at line 24, The import alias in pkg/provision/automount/pvcs.go currently uses v1 for "k8s.io/api/apps/v1" which is inconsistent with other files that use appsv1 and conflicts visually with corev1; change the import alias to appsv1 and update any references/signatures in this file that use v1.Deployment to appsv1.Deployment (ensure compatibility with functions like isAllowedToMount and existsInDeployment in common.go that expect appsv1.Deployment).pkg/provision/automount/configmap.go (1)
23-23: Useappsv1alias for consistency with the rest of the package.
pkg/provision/automount/common.goimportsk8s.io/api/apps/v1asappsv1(see thegetAutomountResourcessignature using*appsv1.Deployment). Using the barev1alias here clashes visually withcorev1and diverges from the sibling file. Preferappsv1for consistency.♻️ Proposed change
- v1 "k8s.io/api/apps/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -func getDevWorkspaceConfigmaps(namespace string, api sync.ClusterAPI, workspaceDeployment *v1.Deployment) (*Resources, error) { +func getDevWorkspaceConfigmaps(namespace string, api sync.ClusterAPI, workspaceDeployment *appsv1.Deployment) (*Resources, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/configmap.go` at line 23, Change the import alias for "k8s.io/api/apps/v1" from v1 to appsv1 in this file and update any occurrences of v1.<Type> (e.g., Deployment) to appsv1.<Type> so the alias matches the rest of the package (same style as getAutomountResources which uses *appsv1.Deployment); this keeps imports consistent with corev1 and sibling files.pkg/provision/automount/gitconfig.go (1)
16-28: Keep project-local imports in the project-local group.
github.com/devfile/devworkspace-operator/pkg/commonis grouped with Kubernetes imports while the other project-local imports are separated below. Move it into the project-local group to match the repository import convention. As per coding guidelines, “Organize imports into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/gitconfig.go` around lines 16 - 28, The import grouping places github.com/devfile/devworkspace-operator/pkg/common with the Kubernetes/third-party group; move that import into the project-local group alongside github.com/devfile/devworkspace-operator/pkg/constants, github.com/devfile/devworkspace-operator/pkg/dwerrors, and github.com/devfile/devworkspace-operator/pkg/provision/sync so imports follow the three-group convention (stdlib, third-party/Kubernetes, project-local) in pkg/provision/gitconfig.go; update the import block accordingly so common is not mixed with appsv1/corev1/k8sclient imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/provision/automount/gitconfig_test.go`:
- Around line 337-343: Tests seed raw volume names which don't match
production-generated names; replace the hard-coded volume names with the
helper-generated names so the "already exists in deployment" branch is
exercised. In gitconfig_test.go update the Deployment Volumes and any other
seeded Volumes (including the other occurrence around the 372-378 block) to use
common.AutoMountSecretVolumeName(constants.GitCredentialsMergedSecretName) for
the secret and
common.AutoMountConfigMapVolumeName(constants.GitCredentialsConfigMapName) for
the configmap so they match production naming.
---
Outside diff comments:
In `@pkg/provision/automount/secret.go`:
- Around line 43-62: Move the early "skip" check so Secrets that are not
eligible to be mounted are bypassed before running validations: call
isAllowedToMount(...) (using mountAs and mountPath from secret.Annotations or a
lightweight/inference of automount target) and continue if it returns false,
before invoking checkAutomountVolumeForPotentialError(...) or
getAccessModeForAutomount(...). Only after isAllowedToMount returns true should
you run checkAutomountVolumeForPotentialError, call getAccessModeForAutomount,
and then build the full automountSecret via getAutomountSecret(...); this
ensures validation errors are not surfaced for Secrets that will not be mounted.
---
Duplicate comments:
In `@pkg/provision/automount/configmap.go`:
- Around line 62-67: When isAllowedToMount rejects the recomputed automount
(from getAutomountConfigmap) for an existing running workspace, don't drop the
resource; instead detect that the ConfigMap/Secret/PVC already exists in
workspaceDeployment by matching on the cluster-side identifier (e.g.,
AutoMountConfigMapVolumeName for ConfigMaps) and carry forward the existing
volume/volumeMount/envFrom from the Deployment into the desired automountCM so
the desired spec matches the running shape; update the logic in
pkg/provision/automount/configmap.go around getAutomountConfigmap and
isAllowedToMount to: if isAllowedToMount returns false but a resource with the
same cluster-side name exists in workspaceDeployment, copy the cluster-side
volume/volumeMount/envFrom into automountCM (and do the same for Secret and PVC
paths) instead of continue’ing, so cross-mode changes (volume ↔ envFrom) don’t
remove existing mounts.
In `@pkg/provision/automount/gitconfig_test.go`:
- Around line 231-234: The test ClusterAPI instances (clusterAPI variables
created as sync.ClusterAPI{ Client:
fake.NewClientBuilder().WithObjects(&testSecret).Build(), Logger: zap.New(), ...
}) leave Ctx nil; update each test setup (all occurrences around the clusterAPI
initializations in gitconfig_test.go) to set Ctx: context.Background() or use a
shared test constructor that returns a sync.ClusterAPI with Ctx populated, and
ensure the test file imports context so client operations receive a non-nil
context like production does.
---
Nitpick comments:
In `@pkg/provision/automount/configmap.go`:
- Line 23: Change the import alias for "k8s.io/api/apps/v1" from v1 to appsv1 in
this file and update any occurrences of v1.<Type> (e.g., Deployment) to
appsv1.<Type> so the alias matches the rest of the package (same style as
getAutomountResources which uses *appsv1.Deployment); this keeps imports
consistent with corev1 and sibling files.
In `@pkg/provision/automount/gitconfig.go`:
- Around line 16-28: The import grouping places
github.com/devfile/devworkspace-operator/pkg/common with the
Kubernetes/third-party group; move that import into the project-local group
alongside github.com/devfile/devworkspace-operator/pkg/constants,
github.com/devfile/devworkspace-operator/pkg/dwerrors, and
github.com/devfile/devworkspace-operator/pkg/provision/sync so imports follow
the three-group convention (stdlib, third-party/Kubernetes, project-local) in
pkg/provision/gitconfig.go; update the import block accordingly so common is not
mixed with appsv1/corev1/k8sclient imports.
In `@pkg/provision/automount/pvcs.go`:
- Line 24: The import alias in pkg/provision/automount/pvcs.go currently uses v1
for "k8s.io/api/apps/v1" which is inconsistent with other files that use appsv1
and conflicts visually with corev1; change the import alias to appsv1 and update
any references/signatures in this file that use v1.Deployment to
appsv1.Deployment (ensure compatibility with functions like isAllowedToMount and
existsInDeployment in common.go that expect appsv1.Deployment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f1a7d53-d922-45d8-8bd9-0d5a2b85f2f7
📒 Files selected for processing (10)
controllers/workspace/devworkspace_controller.gopkg/provision/automount/common.gopkg/provision/automount/common_persistenthome_test.gopkg/provision/automount/common_test.gopkg/provision/automount/configmap.gopkg/provision/automount/gitconfig.gopkg/provision/automount/gitconfig_test.gopkg/provision/automount/pvcs.gopkg/provision/automount/secret.gopkg/provision/automount/templates.go
✅ Files skipped from review due to trivial changes (1)
- pkg/provision/automount/templates.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/provision/automount/common_persistenthome_test.go
- controllers/workspace/devworkspace_controller.go
- pkg/provision/automount/common.go
- pkg/provision/automount/common_test.go
…tart Signed-off-by: Anatolii Bazko <abazko@redhat.com>
…tart Signed-off-by: Anatolii Bazko <abazko@redhat.com>
rohanKanojia
left a comment
rohanKanojia
left a comment
There was a problem hiding this comment.
I've tested all scenarios and can confirm they work as expected ✅ . Just minor nitpicks
|
|
||
| * `controller.devfile.io/mount-on-start`: when set to `"true"`, the resource will only be mounted when a workspace starts. If the resource is created while a workspace is already running, it will not be automatically mounted until the workspace is restarted. This prevents unwanted workspace restarts caused by newly created automount resources. This annotation can be applied to configmaps, secrets, and persistent volume claims. | ||
| + | ||
| For git credential secrets (labelled with `controller.devfile.io/git-credential`) and git TLS configmaps (labelled with `controller.devfile.io/git-tls-credential`), the annotation works similarly but applies collectively: since all git credentials are merged into a single mounted secret, if at least one git credential secret (or TLS configmap) lacks the `controller.devfile.io/mount-on-start` annotation, all git credentials (or TLS configmaps) will be mounted, including those marked with `mount-on-start`. |
There was a problem hiding this comment.
| For git credential secrets (labelled with `controller.devfile.io/git-credential`) and git TLS configmaps (labelled with `controller.devfile.io/git-tls-credential`), the annotation works similarly but applies collectively: since all git credentials are merged into a single mounted secret, if at least one git credential secret (or TLS configmap) lacks the `controller.devfile.io/mount-on-start` annotation, all git credentials (or TLS configmaps) will be mounted, including those marked with `mount-on-start`. | |
| For git credential secrets (labelled with `controller.devfile.io/git-credential`) and git TLS configmaps (labelled with `controller.devfile.io/git-tls-credential`), the annotation is evaluated across all git credential resources as a group, not individually. Since all git credentials are merged into a single mounted secret, if at least one git credential secret (or TLS configmap) lacks the `controller.devfile.io/mount-on-start` annotation, all git credentials (or TLS configmaps) will be mounted, including those marked with `mount-on-start`. |
There was a problem hiding this comment.
Maybe we can add a NOTE block:
[NOTE]
====
This annotation must be applied consistently across all git credential resources. If it is missing from any resource, it will not be honored for any of them.
====
| } | ||
| } | ||
|
|
||
| func TestMountGitCredentialWhenMixedMountOnStartSecrets(t *testing.T) { |
There was a problem hiding this comment.
Does it make sense to add a similar Mixed mount test for git TLS config, too, i.e., TestMountGitTLSConfigWhenMixedMountOnStartConfigMaps?
| assert.Equal(t, common.AutoMountPVCVolumeName("test-pvc"), testPodAdditions.Volumes[0].Name) | ||
| } | ||
|
|
||
| func TestShouldNotMountConfigMapWithMountOnStartWhenRunningAndNotInDeployment(t *testing.T) { |
There was a problem hiding this comment.
This test looks very similar to TestShouldNotMountConfigMapWithMountOnStartIfWorkspaceStarted.
Just wondering if there's a subtle difference I'm missing.
4 participants
What does this PR do?
This PR prevents automatic workspace restarts when mounting resources (ConfigMaps, Secrets, PVC) by introducing a new
controller.devfile.io/mount-on-startattribute.What issues does this PR fix or reference?
eclipse-che/che#23553
Is it tested? How?
Clean namespace before each scenario.
Scenario 1
Scenario 2
/.git-credentials/credentialsScenario 3
/etc/gitconfigScenario 4
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
New Features
Tests
Chore